-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse client_certificate_url
extension and support CertificateURL message
#112
Parse client_certificate_url
extension and support CertificateURL message
#112
Conversation
…ATE_URL extension
…when we serialize it, we get the extension_data.length bytes (if only '\x00\x00') properly
…ructs containing CertificateURL messages
client_certificate_url
message and support CertificateURL messageclient_certificate_url
extension and support CertificateURL message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks great!
A couple of inline comments that kind of address the questions you asked.
Here's some more on two of those questions:
When an extension’s extendion_data is supposed to be empty (e.g. client_certificate_url), should it still be mentioned in tls._construct.Extension’s EnumSwitch? What I have now works, but I think @markrwilliams's idea of creating a helper extension-builder in tls._constructs will be useful here.
I think it could be. Let's write it up in a new PR and how it goes!!
@reaperhulk had raised a good point when I was working on the TLS PRF that being too specific with the errors and exceptions is not always a good idea. Same could be true for the InvalidPaddingException
I definitely don't want to leak sensitive data. As I mentioned in review comment, though, I really want tls
to allow for meaningful error reporting. In particular I'd like tls
to be usable as a library for people investigating TLS issues. openssl isn't great for such a use case :(
I think a good compromise is to start developing an exception hierarchy. We can start with a root TLSException
from which all other exceptions inherit. Lower-level parsing code, like we're working on these days, can then raise very specific exceptions that higher-level code can always catch with except TLSError:
. That code can then raise a new exception that contains suitable information. This would hopefully end up looking like Python 3's improved OSError
hierarchy.
(Inheritance is of course not great, but Python's except
mechanism seems to kind of expect it.)
|
||
|
||
class InvalidPaddingException(Exception): | ||
# XXX: Is leaking this info in CertificateURL (i.e. revealing the specific |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that at this level the code should be as specific as possible. I'd really like tls
to have the ability to do better than OpenSSL alert handshake failure
.
Upper level code can always suppress these exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, thanks! I'll add the TLSException
definition, and spin up a new small PR to update UnsupportedExtensionException
and UnsupportedCipherException
to inherit from it as well.
padding = attr.ib() | ||
sha1_hash = attr.ib() | ||
|
||
# XXX: The "padding" byte MUST be 0x01. It is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use an attrs validator here, as it'd cover both serialization and deserialization.
But since this is a parsing issue, it might be nice to fail early when reading potentially malicious data. You can use construct.OneOf
to do that:
>>> construct.OneOf(construct.UBInt8('a'), [1]).parse('\x01')
1
>>> construct.OneOf(construct.UBInt8('a'), [1]).build(1)
'\x01'
>>> construct.OneOf(construct.UBInt8('a'), [1]).parse('\x02')
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 188, in parse
return self.parse_stream(BytesIO(data))
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 198, in parse_stream
return self._parse(stream, Container())
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 288, in _parse
return self._decode(self.subcon._parse(stream, context), context)
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 412, in _decode
raise ValidationError("invalid object", obj)
construct.adapters.ValidationError: ('invalid object', 2)
>>> construct.OneOf(construct.UBInt8('a'), [1]).build(2)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 212, in build
self.build_stream(obj, stream)
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 219, in build_stream
self._build(obj, stream, Container())
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/core.py", line 290, in _build
self.subcon._build(self._encode(obj, context), stream, context)
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 415, in _encode
return self._decode(obj, context)
File "/home/mrw/.virtualenvs/tls/local/lib/python2.7/site-packages/construct/adapters.py", line 412, in _decode
raise ValidationError("invalid object", obj)
construct.adapters.ValidationError: ('invalid object', 2)
Note that failing early on parsing means failing late on serialization. Maybe it's not a bad idea to have an attrs validator and construct.OneOf
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is useful! No obvious thoughts on failing early/late, let me play around with this and see what the tests look like. We can do multiple iterations on this until everyone is happy! Updates coming soon!
@@ -45,6 +45,8 @@ class HandshakeType(Enum): | |||
CERTIFICATE_VERIFY = 15 | |||
CLIENT_KEY_EXCHANGE = 16 | |||
FINISHED = 20 | |||
CERTIFICATE_URL = 21 | |||
CERTIFICATE_STATUS = 22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think a later PR can make the Handshake
construct use EnumSwitch
to dispatch to the various handshake types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!! I'll play around with that next, blends in well with the #106 branch I was thinking of tackling.
… TLSValidationException that's not a ConstructError so that Range implementation (and hence TLSPrefixedArray) doesn't break
2 similar comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments and well-deserved praise. I'm most particular about the test against the empty dict.
@@ -211,6 +213,35 @@ def EnumSwitch(type_field, type_enum, value_field, value_choices, # noqa | |||
default=default)) | |||
|
|||
|
|||
class TLSExprValidator(construct.Validator): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great way to implement TLSOneOf
! We can reuse TLSExprValidator
for other constructs. Thanks!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:)
Took this idea from construct
's OneOf
.
:py:class:`tls.exceptions.TLSValidationException` instead of a | ||
``ConstructError`` subclass on mismatch. | ||
|
||
This is necessary because any ConstructError signifies the end of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to put an explanation like this into TLSExprValidator
's doc string.
Tests for :py:class:`tls._common._constructs.TLSExprValidator`. | ||
""" | ||
@pytest.fixture | ||
def data_class(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good name - no need for # noqa
. I think we should use this approach more!
assert len(record.extensions) == 1 | ||
assert (record.extensions[0].type == | ||
enums.ExtensionType.CLIENT_CERTIFICATE_URL) | ||
assert record.extensions[0].data == {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this be
assert record.extensions[0].data == Container()
Since data
is actually a Container
and I prefer to forget that Container
subclasses dict
.
…g to '{}' (Container subclasses dict)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
A few things I'd love to hear thoughts on:
What is the correct place to validate the padding byte? Should/Can this be moved to
tls.constructs
and or be cleaner than what I have now?When an extension’s
extension_data
is supposed to be empty (e.g. client_certificate_url), should it still be mentioned intls._construct.Extension
’s EnumSwitch? What I have now works, but I think @markrwilliams's idea of creating a helper extension-builder intls._constructs
will be useful here.I've put a couple of
XXX
s in the code that could use some discussion. @reaperhulk had raised a good point when I was working on the TLS PRF that being too specific with the errors and exceptions is not always a good idea. Same could be true for theInvalidPaddingException